Skip to content

Conversation

@nishu-murmu
Copy link

This is being implemented in response to this issue from WXT. wxt-dev/wxt#1569

Copy link
Owner

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting back to github after my long break. Couple of changes on this PR then it's good to go.

const onClicked =
defineEventWithTrigger<(tab: Tabs.Tab, info: Action.OnClickData | undefined) => void>();

let DEFAULT_BADGE_BACKGROUND_COLOR = '#5F5D5B';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be const instead of let since they're never reassigned.

badgeBackgroundColorState.windows.clear();

titleState.global = '';
titleState.tabs.clear();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not resetting the badgeTextColorState. Add:

badgeTextColorState.global = DEFAULT_BADGE_TEXT_COLOR;
badgeTextColorState.tabs.clear();
badgeTextColorState.windows.clear();

const result = await fakeBrowser.action.getBadgeBackgroundColor({ tabId });
expect(result).toEqual([0, 255, 0, 255]);
});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description says "default black" but #5F5D5B is actually rgb(95, 93, 91) which is a gray color. Update to "should fallback to default gray if color not set" or similar.

Comment on lines +70 to +90
});

describe('setBadgeTextColor / getBadgeTextColor', () => {
it('should set and get global badge text color', async () => {
const color = '#0000FF';
fakeBrowser.action.setBadgeTextColor({ color });
//@ts-ignore
fakeBrowser.action.getBadgeTextColor({}, result => {
expect(result).toEqual(color);
});
});

it('should set and get tab-specific badge text color', async () => {
const tabId = 123;
const color = '#00FFFF';
fakeBrowser.action.setBadgeTextColor({ tabId, color });
//@ts-ignore
fakeBrowser.action.getBadgeTextColor({ tabId }, result => {
expect(result).toBe(color);
});
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with these @ts-ignores? Is it a conflict with @types/webextension-polyfill vs chrome?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants